o/snapstate, o/devicestate: update hooks to let devicestate own what results in a seed-refresh - #17006
Merged
sergiocazzolato merged 2 commits intoMay 25, 2026
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #17006 +/- ##
==========================================
- Coverage 79.14% 79.04% -0.10%
==========================================
Files 1366 1369 +3
Lines 192385 191772 -613
Branches 2465 2465
==========================================
- Hits 152266 151590 -676
- Misses 30998 31038 +40
- Partials 9121 9144 +23
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Mon May 25 17:52:04 UTC 2026 Failures:Preparing:
Executing:
Skipped tests from snapd-testing-skipIf you wish to have any of the below tests run in your PR, in your PR description, add 'unskip:' followed by a copy-and-pasted list (without variants) of the below tests you wish to run (unskip plus test list must be valid yaml)
|
pedronis
reviewed
May 1, 2026
pedronis
left a comment
pedronis
left a comment
Contributor
There was a problem hiding this comment.
as discussed this needs some cleaning up
| } | ||
| added[snapsup.SnapName()] = true | ||
|
|
||
| if !snapsup.ComponentExclusiveOperation { |
Contributor
There was a problem hiding this comment.
as discussed this shows that the responsibility boundary crossing here is not great, some of this building up of sets of snapups and compups belongs to snapstate really
Member
Author
There was a problem hiding this comment.
I've reworked this a bit, hopefully things are better now.
andrewphelpsj
force-pushed
the
seed-refresh-devicestate-ownership
branch
3 times, most recently
from
May 6, 2026 18:14
65fa590 to
a4502a7
Compare
andrewphelpsj
force-pushed
the
seed-refresh-devicestate-ownership
branch
from
May 11, 2026 08:43
a4502a7 to
4ab7583
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the seed-refresh integration so that devicestate (rather than snapstate) owns the policy for which snap refreshes participate in a seed refresh, and also owns updating an in-flight seed-refresh change with late prerequisite candidates. This aligns with the stated goal of centralizing “what results in a seed-refresh” in devicestate ahead of adding more selection criteria in a follow-up PR.
Changes:
- Introduces a
snapstate.SeedRefreshCandidatedata model and changes theSeedRefreshTaskshook to accept candidates and return which snaps were selected. - Adds a new
snapstate.UpdateSeedRefreshChangehook so devicestate can update an existing seed-refresh change with late prerequisite candidates. - Updates seed-refresh-related tests to assert on the new candidate flow and hook behavior (including component-exclusive candidates).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| overlord/snapstate/snapstate_update_test.go | Updates seed-refresh update tests to use candidate-based hooks and validate initial/prerequisite candidate selection. |
| overlord/snapstate/snapstate_test.go | Adds shared test hook helpers (mockSeedRefreshHooks) and installs them in test setup. |
| overlord/snapstate/seed.go | Refactors snapstate seed-refresh wiring to pass candidate structs into devicestate and to merge late prerequisites via the new hook. |
| overlord/snapstate/reboot_test.go | Adjusts reboot/seed-refresh tests to validate component-exclusive candidate behavior with the new hook API. |
| overlord/snapstate/handlers.go | Updates prerequisite-refresh flow to use the updated seed-refresh merge helper signature. |
| overlord/devicestate/devicestate.go | Implements candidate filtering/selection and late-change updates in devicestate; wires new hook in cross-manager init. |
| overlord/devicestate/devicestate_test.go | Updates devicestate unit tests for the new SeedRefreshTasks signature and adds coverage for UpdateSeedRefreshChange. |
| overlord/devicestate/devicestate_systems_test.go | Updates systems test to use the new SeedRefreshTasks signature and assert selected snaps. |
Comment on lines
232
to
234
| // seed-refresh task graph by adding its setup tasks to create-recovery-system, | ||
| // joining its task set to the seed-refresh lanes, and ensuring seed creation | ||
| // tasks depend on the prerequisite refresh tasks. |
Contributor
There was a problem hiding this comment.
maybe indeed the comment should mention the bi of logic in devicestate that this is using?
andrewphelpsj
commented
May 18, 2026
andrewphelpsj
left a comment
andrewphelpsj
left a comment
Member
Author
There was a problem hiding this comment.
Comments from in-person review in the sprint.
| InstanceName string | ||
| // SnapSetupTasks are the snap tasks that should be considered as inputs to | ||
| // recovery system creation. Will be empty for component-only refreshes. | ||
| SnapSetupTasks []string |
Member
Author
There was a problem hiding this comment.
Add a re-refresh test to cover the double create-recovery-system case
Member
Author
There was a problem hiding this comment.
I checked, we have TestUpdateWithGoalSeedRefreshReRefreshCreatesSecondSeed which creates this scenario, and then the tests in devicestate that actually cover skipping already done seed-creation tasks.
andrewphelpsj
force-pushed
the
seed-refresh-devicestate-ownership
branch
from
May 18, 2026 14:23
4ab7583 to
4653a39
Compare
pedronis
approved these changes
May 20, 2026
Comment on lines
232
to
234
| // seed-refresh task graph by adding its setup tasks to create-recovery-system, | ||
| // joining its task set to the seed-refresh lanes, and ensuring seed creation | ||
| // tasks depend on the prerequisite refresh tasks. |
Contributor
There was a problem hiding this comment.
maybe indeed the comment should mention the bi of logic in devicestate that this is using?
bboozzoo
approved these changes
May 20, 2026
| return setTaskRecoverySystemSetup(create, setup) | ||
| } | ||
|
|
||
| func seedRefreshIncludesSnap(dctx snapstate.DeviceContext, instanceName string) bool { |
Contributor
There was a problem hiding this comment.
would this check make more sense in snapstate? or maybe it could be exported to avoid the awkwardness or returning nil in UpdateSeedRefreshChange if snap isn't part of the seed refresh?
Member
Author
There was a problem hiding this comment.
Talked on MM, but the main motivations for this living here is because we'll need to check the seed to make this decisions, which would be very hard/awkward to do from snapstate.
andrewphelpsj
force-pushed
the
seed-refresh-devicestate-ownership
branch
2 times, most recently
from
May 20, 2026 15:42
2dbd0e4 to
7510e18
Compare
andrewphelpsj
added
Run nested
andrewphelpsj
force-pushed
the
seed-refresh-devicestate-ownership
branch
from
May 21, 2026 14:39
7510e18 to
dc3a6c7
Compare
andrewphelpsj
force-pushed
the
seed-refresh-devicestate-ownership
branch
from
May 22, 2026 12:05
dc3a6c7 to
f62d38b
Compare
ZeyadYasser
pushed a commit
to ZeyadYasser/snapd
that referenced
this pull request
Jun 3, 2026
…results in a seed-refresh (canonical#17006) * overlord/snapstate: migrate seed-refresh TODO tests * o/snapstate, o/devicestate: update hooks to let devicestate own what results in a seed-refresh
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This moves some code around so that
devicestateowns more about what results in a seed refresh. Next PR will make it so that we consider snaps that are present in the seed, as well as what snaps are in the model, when deciding whether or not to refresh the seed.